Repository navigation
test(purchase): pin the Azure savings-plans alias to the re-drive token through the real client - #1305
Conversation
…en through the real client The LeanerCloud#838 matrix (written against cloud-commitments-go ad67030387e4) says the Azure savings-plans client names its order alias from time.Now().UnixNano(), so the platform's derived idempotency token never reaches Azure. The module pinned in go.mod now (providers/azure 3def3f898617) derives the alias from the token via common.OrderIDFromToken, but nothing on the platform side pinned that: TestRedriveReusesDerivedTokensForEveryProvider stops at a mock service client and does not cover azure savings-plans. Run the platform purchase fan-out against the real pinned savings-plans client with only the order-alias API faked, and assert that each recommendation PUTs OrderIDFromToken(DeriveIdempotencyToken(lineage, index)), that a re-drive PUTs the identical names, that recommendations and lineages do not collide, and that the platform still refuses Azure savings-plans re-drive (whether Azure collapses a repeat alias PUT is server behavior no fake can prove). Tests only; no production change. Refs LeanerCloud#838 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB8. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB12. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on |
…alias proof Main's LeanerCloud#1305 added a test that calls processPurchaseRecommendations with the old five-argument signature. This branch adds the awsPartition argument, so after the merge the purchase test package no longer compiled. The account in that test is Azure, so it passes "" like the branch's other non-AWS call sites. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…purchases (#1248) * chore(deps): bump cloud-commitments-go providers/aws to 92722e6 for the aws-cn Savings Plans refusal Pins providers/aws to the cloud-commitments-go#566 merge commit, which makes Savings Plans offering lookups, validation and purchases fail with savingsplans.ErrUnsupportedPartition before any AWS call when a region they see is cn-* (aws-cn bills in CNY and the purchase path is USD-only). MVS moves pkg to ff6225d, the version that commit requires. Refs #1194 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(accounts): return an unsupported-partition notice for aws-cn accounts aws-cn accounts were onboarded with no hint that CUDly cannot buy Savings Plans for them. Account responses (create, update, get, list and the self-account) now carry a derived, non-persisted "notices" list; an AWS account whose role ARN is in aws-cn (or a self-account when CUDly itself runs in aws-cn) gets an "unsupported_partition" notice. Onboarding is still allowed so existing aws-cn accounts are not stranded. The ARN partition parsing moves to config.AWSPartitionFromARN so the account model and the STS host-identity path share it. Refs #1194 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(purchase): refuse aws-cn Savings Plans purchases before any AWS call A Savings Plans rec for an account whose role ARN is in aws-cn now fails with an error wrapping savingsplans.ErrUnsupportedPartition before a provider is built, on both the plan fan-out and the single-account (direct-execute) paths. Other recs in the same execution still run, so a mixed run ends partially_completed as before. The library's region-based guard stays as the backstop for accounts with no role ARN. Closes #1194 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(api): put the host account ID on an allowlisted fixture line git-secrets' AWS account-ID detector flagged the inline sourceIdentity literal in the partition notice test, which failed the pre-commit hook and the allowlist script's scan of all tracked files. Split the literal one field per line so the placeholder sits on an `AccountID: "..."` line that .gitallowed already permits in test files. Same value, same assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(purchase): pass the account partition in the Azure savings-plans alias proof Main's #1305 added a test that calls processPurchaseRecommendations with the old five-argument signature. This branch adds the awsPartition argument, so after the merge the purchase test package no longer compiled. The account in that test is Azure, so it passes "" like the branch's other non-AWS call sites. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
What #838 asked, and what the pinned module actually does now
#838's matrix was written against cloud-commitments-go
ad67030387e4. Its open gap was that the Azure savings-plans client names its order alias fromtime.Now().UnixNano(), so the platform'sDeriveIdempotencyToken(lineage, index)never reaches Azure.I re-checked that against the module go.mod pins now (
providers/azure v0.0.0-20261017134002-3def3f898617). That row is out of date.services/savingsplans/client.go:376now does:So with a token (the platform's purchase path), the alias is the deterministic GUID derived from the token. The timestamp name is used only with an empty token (the CLI path), and a malformed token fails before any PUT. The module tests this with a mock in
TestPurchaseCommitment_DeterministicAliasNameFromToken.The gap in the platform: nothing here pinned it.
TestRedriveReusesDerivedTokensForEveryProviderstops at a mock service client and does not cover azure savings-plans at all. A go.mod bump that brought the timestamp alias back would pass every platform test.Change (tests only, no production change)
New file:
internal/purchase/azure_savings_plans_alias_proof_test.go, withTestAzureSavingsPlansAliasIsDerivedFromTheRedriveToken. It runsprocessPurchaseRecommendations(the platform's real per-rec fan-out and token derivation) against the real pinnedsavingsplans.Client. Only the Azure order-alias API (SetOrderAliasClient) is faked. For both dispatchable slugs (savings-plansandsavingsplans) it asserts:OrderIDFromToken(DeriveIdempotencyToken(lineage, index));cudly-<nanos>timestamp fallback;common.ServiceSavingsPlansAll, the only service type Azure routes to this client;recRedriveRefusalReasonstill refuses Azure savings-plans re-drive (see below).Why the platform keeps refusing Azure savings-plans re-drive
A deterministic alias is necessary but not sufficient. Whether Azure answers a second
PUT savingsPlanOrderAliases/{name}by returning the first order is server behavior. That could mean neither a second plan nor an error, and no fake can prove it. Neither repo has a test or a cited provider doc for it. So this PR keeps option (a) from the issue: refuse automatic re-drive and Retry for Azure savings plans. That coversallRecsSafeToRedrive, the #1668 Retry gate and the #1718armedRedriveRefusalbackstop, all of which go throughrecRedriveRefusalReason. The new test fails if someone relaxes that as a side effect.Remaining exposure, unchanged: a first attempt whose response is lost. The row fails, and a human who then starts a new purchase gets a new lineage, so a new alias and a second plan. That is a legitimate new buy from the platform's point of view.
Proposed cloud-commitments-go change (not made here; go.mod untouched)
This would let the owner choose option (b) safely. In
providers/azure/services/savingsplans/client.goPurchaseCommitment, whenopts.IdempotencyToken != "", first callGET savingsPlanOrderAliases/{aliasName}. That isarmbillingbenefits.SavingsPlanOrderAliasClient.Get, which exists in the pinnedarmbillingbenefits v1.0.0; addGetto theSavingsPlanOrderAliasAPIinterface andrealOrderAliasClient.DoIdempotentPurchaseTwoSteplookup.BeginCreate.Add module tests for each branch, including "existing alias, so BeginCreate is never called". After that lands and go.mod is bumped, the platform can drop the
azure/savingsplanscase inrecRedriveRefusalReason. It would update the existing guard tests (TestRedriveGuardRefusalSetIsExactlyTheDispatchableSpellings,TestRedriveRefusedWhereNoProviderGuardExists) and the last assertion of the new test.Matrix re-verified at the current pins
The pins are
providers/aws aeb8a6f7ff51,providers/azureandproviders/gcp 3def3f898617, andpkg 541e05e5fd29. I read the module source in the Go module cache.CreateSavingsPlanInput.ClientToken(savingsplans/client.go:250)TestClient_PurchaseCommitment_SetsClientTokenForIdempotencyIdempotentReservationID+idempotencyGuard/recoverAlreadyExists(e.g. rds/client.go:150,173)*Idempotent*(5 tests), pluspurchase_empty_token_test.goper servicefindRIByIdempotencyTokenbefore buying (ec2/client.go:145,226)guardAgainstRepurchasematches an owned node on offering ID, node count and start time (redshift/client.go:113-139)*Idempotent*reservations.DoIdempotentPurchaseTwoStep(internal/reservations/purchase.go:679)TestFindReservationOrderByIdempotencyToken_*etc.OrderIDFromToken(token)(savingsplans/client.go:376); server-side collapse of a repeat PUT unprovenTestPurchaseCommitment_DeterministicAliasNameFromToken(mock); now also pinned hereRequestIdfromOrderIDFromToken(token)+ deterministic name (computeengine/client.go:829,873-878)TestComputeEngineClient_PurchaseCommitment_IdempotentReDrivePurchaseCommitmentreturns an error (no purchase API)Owner decisions left open (why this is Refs, not Closes)
CreateTagsafter the buy, so a crash between the buy and the tag still leaves an untagged RI. That argues for keeping the refusal there too.Deploy
None needed. This PR is tests only, and the platform behavior is unchanged. The Azure/GCP deploy pause since 2026-10-20 does not affect it.
Coordination with in-flight work
This PR adds one new test file and touches no production lines. Our
fix/issue-243branch already rewrites the stale "UnixNano" comments ininternal/purchase/manager.go(theRedriveRefusalReason/recRedriveRefusalReason/safeFaildoc comments), so I deliberately left them alone here to avoid a conflict. It does not overlap #1111, #1245 or fix/issue-532/-54/-361.Refs #838
Verification
All of the following is unit, mock or fixture evidence. Nothing calls Azure or makes a purchase. The Azure order-alias API is a fake recorder.
Env:
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9, Linux.go build ./...,go vet ./...,go vet -tags integration ./...: OKgo test ./internal/purchase/ -count=1: ok (32.6s)go test ./internal/mocks/ -count=1: okgolangci-lint run --allow-parallel-runners ./internal/purchase/(v2.10.1): 0 issuesgofmt -l internal/purchase/: clean.GOWORK=off go mod tidy -diff: clean (go.mod/go.sum unchanged;armbillingbenefitswas already a direct require).Fail-before and pass-after, by mutation. The test pins existing behavior, so "before" means a regression of each link in the chain:
cudly-<UnixNano>alias, wired in throughGOWORK=off go test -modfile=<tmp>/go.modwith areplace; the repo's go.mod is untouched): FAIL. Expectedc505bf94-f62f-1682-6300-54342dcab999, gotcudly-1792744476476032937; the re-drive alias differs; "alias … is the timestamp fallback". Both subtests fail.execution.goderives the token fromexec.ExecutionIDinstead ofidempotencyLineageKey(exec)): FAIL, "each purchase PUTs the alias derived from DeriveIdempotencyToken(lineage, index)".recRedriveRefusalReasonazure savings-plans case disabled): FAIL, "Azure savings-plans re-drive must stay refused until a repeat alias PUT is proven to dedupe".go test ./internal/purchase/ -count=1 -run TestAzureSavingsPlansAliasIsDerivedFromTheRedriveToken -v: PASS (both subtests).Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@b56afb2b= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-324andfix/issue-204both edit.github/workflows/ci.ymlnext to theazure-kv-access-policyjob. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran oned0c7174, which differs fromb56afb2bonly by one.gitallowedline on fix/issue-324:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout):internal/api,internal/purchase,internal/schedulerandinternal/serverall pass..gitallowedline. Re-run atb56afb2b: git-secrets, using the repo'sscripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
Get(above) belongs in cloud-commitments-go.Maintainer: please mirror #838's triage labels onto this PR:
priority/p2,severity/medium,type/chore,triaged,urgency/this-quarter,impact/few,effort/m.🤖 Generated with Claude Code